Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing WindowOperator#isBlocked method #468

Merged
merged 1 commit into from
Mar 14, 2019

Conversation

sopel39
Copy link
Member

@sopel39 sopel39 commented Mar 13, 2019

WindowOperator can block when spilling,
therefore it needs to explicitly support isBlocked
method.

@sopel39 sopel39 requested a review from findepi March 13, 2019 11:05
@cla-bot cla-bot bot added the cla-signed label Mar 13, 2019
@findepi
Copy link
Member

findepi commented Mar 13, 2019

WindowOperator can block when spilling,
therefore it needs to explicitly support isBlocked
method.

https://github.com/prestosql/presto/blob/c74b775e103a7a239aafef1b7ca15fe6370495fe/presto-main/src/main/java/io/prestosql/operator/Operator.java#L64-L66

@sopel39
Copy link
Member Author

sopel39 commented Mar 13, 2019

@findepi spill can be triggered by window operator itself when flipping memory without explicit revoke signal

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be feasible to have a test?

return outputPages.getBlockedFuture();
}

return NOT_BLOCKED;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regret that you cannot simply return outputPages.getBlockedFuture().

add a comment:

// We can block e.g. because of self-triggered spill
if (outputPages.isBlocked()) {

WindowOperator can block when spilling,
therefore it needs to explicitly support isBlocked
method.
@sopel39 sopel39 merged commit 517765b into trinodb:master Mar 14, 2019
@sopel39 sopel39 deleted the ks/add_is_block_window branch March 14, 2019 10:51
@sopel39 sopel39 mentioned this pull request Mar 14, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants